OCPBUGS-55192: Add IngressController .spec.domain validation #2308
Conversation
|
Hello @grzpiotrowski! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: grzpiotrowski The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
grzpiotrowski
changed the title
openshift-ci-robot
added
jira/severity-critical
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-55192, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
grzpiotrowski
force-pushed
the
OCPBUGS-55192-ic-spec-domain-validation
branch
from
2e10452 to
a538d6c
Compare
openshift-ci
bot
added
size/L
grzpiotrowski
force-pushed
the
OCPBUGS-55192-ic-spec-domain-validation
branch
from
a538d6c to
d5e7974
Compare
openshift-ci
bot
added
size/M
grzpiotrowski
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
grzpiotrowski
force-pushed
the
OCPBUGS-55192-ic-spec-domain-validation
branch
from
d5e7974 to
a179eae
Compare
openshift-ci
bot
added
size/L
grzpiotrowski
force-pushed
the
OCPBUGS-55192-ic-spec-domain-validation
branch
from
a179eae to
c4e9ebf
Compare
This commit fixes OCPBUGS-55192. https://issues.redhat.com/browse/OCPBUGS-55192 Add ratcheting validation of the .spec.domain field of ingress controller. Domain must consist of lowercase alphanumeric characters '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters. * operator/v1/types_ingress.go (IngressControllerSpec): Add ratcheting validation of the Domain field. * operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml Add test cases for the ingress controller .spec.domain field validation Generated files: * operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml * operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml * operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerLBSubnetsAWS.yaml * operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/SetEIPForNLBIngressController+IngressControllerLBSubnetsAWS.yaml * operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/SetEIPForNLBIngressController.yaml
grzpiotrowski
force-pushed
the
OCPBUGS-55192-ic-spec-domain-validation
branch
from
c4e9ebf to
9a9eb85
Compare
|
/retitle OCPBUGS-55192: Add IngressController .spec.domain validation |
openshift-ci
bot
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@grzpiotrowski: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-55192, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/assign |
| @@ -68,6 +68,7 @@ type IngressControllerSpec struct { | |||
| // | |||
| // If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. | |||
| // | |||
| // +kubebuilder:validation:XValidation:rule="(has(oldSelf) && self == oldSelf) || self.matches('^([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)'+'(\\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$')",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" | |||
There was a problem hiding this comment.
Which releases are you planning to fix this bug in? With ratcheting validation in the API server you shouldn't need (has(oldSelf) && self == oldSelf) anymore, but we can test that
| @@ -68,6 +68,7 @@ type IngressControllerSpec struct { | |||
| // | |||
| // If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. | |||
| // | |||
| // +kubebuilder:validation:XValidation:rule="(has(oldSelf) && self == oldSelf) || self.matches('^([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)'+'(\\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$')",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" | |||
There was a problem hiding this comment.
You don't actually explain a label here right? Probably want to rework the first part of the error message to explain that it's alphanumeric and possibly hypehanted labels, separate by periods
| @@ -68,6 +68,7 @@ type IngressControllerSpec struct { | |||
| // | |||
| // If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. | |||
| // | |||
| // +kubebuilder:validation:XValidation:rule="(has(oldSelf) && self == oldSelf) || self.matches('^([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)'+'(\\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$')",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" | |||
There was a problem hiding this comment.
Opted not to add the max length validation // +kubebuilder:validation:MaxLength=253 as it could be slightly misleading, because even if .spec.domain does not exceed that 253 characters limit, we could end up with an invalid canonical hostname error as this is constructed from the router name and the IC domain, which length could not be predicted here.
What is the minimum length of the router name? From that you can work out the actual maximum of this, but you're right, it won't be 100% accurate. It's still better than nothing though. We should add a max length here
| @@ -68,6 +68,7 @@ type IngressControllerSpec struct { | |||
| // | |||
| // If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. | |||
| // | |||
| // +kubebuilder:validation:XValidation:rule="(has(oldSelf) && self == oldSelf) || self.matches('^([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)'+'(\\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$')",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" | |||
There was a problem hiding this comment.
Have you considered using a format validation?
| // +kubebuilder:validation:XValidation:rule="(has(oldSelf) && self == oldSelf) || self.matches('^([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)'+'(\\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$')",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" | |
| // +kubebuilder:validation:XValidation:rule="!format.dns1123Subdomain.validate(self).hasValue()",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" |
| @@ -68,6 +68,7 @@ type IngressControllerSpec struct { | |||
| // | |||
| // If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. | |||
| // | |||
| // +kubebuilder:validation:XValidation:rule="(has(oldSelf) && self == oldSelf) || self.matches('^([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)'+'(\\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$')",message="domain must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters" | |||
There was a problem hiding this comment.
While we are here, can we update the godoc to more thoroughly explain the validations we are imposing?
This PR fixes OCPBUGS-55192.
Add ratcheting validation of the .spec.domain field of ingress controller.
Domain must consist of lowercase alphanumeric characters '-' or '.', and each label must start and end with an alphanumeric character.
Previously, the user could configure the .spec.domain field incorrectly which could result in the router pods entering a
CrashLoopBackOffstate immediately upon creation witherror: invalid canonical hostname: [...].Opted not to add the max length validation
// +kubebuilder:validation:MaxLength=253as it could be slightly misleading, because even if.spec.domaindoes not exceed that 253 characters limit, we could end up with an invalid canonical hostname error as this is constructed from the router name and the IC domain, which length could not be predicted here.